Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix expo config plugin #161

Merged
merged 2 commits into from
Mar 5, 2024
Merged

fix expo config plugin #161

merged 2 commits into from
Mar 5, 2024

Conversation

ottob
Copy link
Contributor

@ottob ottob commented Feb 23, 2024

But there is still an error:

Error: Cannot find module '../../package.json'
Require stack:
- /myproject/node_modules/@intercom/intercom-react-native/lib/commonjs/expo-plugins/index.js

since

import packageJson from '../../package.json';

should be

import packageJson from '../../../package.json';

because after the build the file ends up in /lib/commonjs/expo-plugins/index.js so it needs to step back three folders.
not sure how to fix that...

@ottob ottob mentioned this pull request Feb 23, 2024
@ansh
Copy link

ansh commented Feb 23, 2024

Yeah this is broken and should be merged ASAP @uddish

@tomfinney
Copy link

tomfinney commented Feb 28, 2024

the only way i can think to make this work (quickly...) is to just completely remove importing the package.json from inside of the expo plugins file.

import packageJson from '../../package.json';

we just can't have a relative import like that if src/expo-plugins/index.ts is going to end up nested in another directory

so, my great suggestion would be to just remove import package.json there, at the bottom of the expo-plugins/index.ts file, change it to be like

const thing = (pkg) => createRunOncePlugin(
  withIntercomReactNative,
  pkg.name,
  pkg.version
);

export default thing

then in side of the app.plugin.js file, we import the package json inside of there and pass the stuff along like

const packageJson = require('./package.json');

const pkg = {
  // Prevent this plugin from being run more than once.
  // This pattern enables users to safely migrate off of this
  // out-of-tree `@config-plugins/intercom-react-native` to a future
  // upstream plugin in `intercom-react-native`
  name: packageJson.name,
  // Indicates that this plugin is dangerously linked to a module,
  // and might not work with the latest version of that module.
  version: packageJson.version,
};

const plugin = require('./lib/commonjs/expo-plugins');

module.exports = plugin.default(pkg);

@ottob
Copy link
Contributor Author

ottob commented Feb 28, 2024

the only way i can think to make this work (quickly...) is to just completely remove importing the package.json from inside of the expo plugins file.

Thanks, this sounds like a good solution. I tried to push commits for this now.

@ottob ottob changed the title almost fix expo plugin fix expo config plugin Feb 28, 2024
@tomfinney
Copy link

the only way i can think to make this work (quickly...) is to just completely remove importing the package.json from inside of the expo plugins file.

Thanks, this sounds like a good solution. I tried to push commits for this now.

👏

seems alright to me

there's probably a less dumb way of doing it but the app.plugin.js file itself referring to a lib/commonjs file that only exists after being built makes it a bit tricky... 🤷

@renatoalvesmh
Copy link

Could we merge it ? im having issues with sdk 50

@renatoalvesmh
Copy link

@ottob seems is failing on lint step in CI
just an small fix, would be possible to merge it ?
We are having issues in our company with new version would be amazing if its could be merged

get rid of relative import path of package.json

idea from @tomfinney
@ottob
Copy link
Contributor Author

ottob commented Mar 4, 2024

just an small fix, would be possible to merge it ?

fixed the lint issue now

@renatoalvesmh
Copy link

@ottob thank you so much
just missing to merge and commit new version in npm

@renatoalvesmh
Copy link

Hello
@ottob @uddish any plans to release it ?
Thanks

@CarlMenke
Copy link

My boss is waiting for a feature in our app that wont work until this is merged, you guys are life savers if you can get it merged today or tomorrow!

@uddish
Copy link
Contributor

uddish commented Mar 4, 2024

Hey @ottob
Thanks for the PR. Were you able to test it locally?

@ottob
Copy link
Contributor Author

ottob commented Mar 4, 2024

Were you able to test it locally?

No, but I have basically the same change locally, and it works. And it can not get more broken than it is :)

And I can test it directly when you release a new version.

@ottob
Copy link
Contributor Author

ottob commented Mar 4, 2024

Ive tested it now locally, and it works for me.

@renatoalvesmh
Copy link

renatoalvesmh commented Mar 5, 2024

@uddish @ottob Thank you! Would be great if it could be released

Copy link
Contributor

@MikeMcNamara MikeMcNamara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for your work on this folks. Will get this merged in now.

@MikeMcNamara MikeMcNamara merged commit a8d20aa into intercom:main Mar 5, 2024
6 checks passed
@renatoalvesmh
Copy link

Thank you so much @MikeMcNamara
Some plans to bump version in npm ?

@MikeMcNamara
Copy link
Contributor

Version has been bumped now in npm!

@hanyufoodles
Copy link

I just tested the plugin with 6.8.0 and it works! I had just to replace config-plugin-react-native-intercom by @intercom/intercom-react-native.

Thank you all for your work! ❤️ we can finally to switch to an official up-to-date plugin!

@ottob ottob deleted the patch-1 branch March 5, 2024 20:50
@ottob
Copy link
Contributor Author

ottob commented Mar 5, 2024

@MikeMcNamara two steps forward, one step back 😀

#171

@jamesedmonston
Copy link

Thanks for this!

uddish pushed a commit that referenced this pull request Mar 13, 2024
get rid of relative import path of package.json

idea from @tomfinney

Co-authored-by: Mike McNamara <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants